Add configuration options to HTTP server #9765

vtnerd · 2025-02-01T22:14:21Z

No description provided.

selsta · 2025-02-02T08:57:46Z

What would be a scenario where someone changes rpc-response-soft-limit?

selsta · 2025-02-02T09:01:44Z

This seems to break multiple functional tests, not sure if it's the IP limit or the response size limit.

iamamyth · 2025-02-02T09:19:08Z

The RPC client used in those tests does a poor job of connection management: Depending on the exact version of the requests library in use, it either opens and closes a new connection for every request (recent versions of the library), or re-uses a global, "default" session, which makes it tricky to clean up. I'd check the IP limit, and also, it might be appropriate to exempt local subnets.

Edit: Based on the trace and the build setup, I expect the connection per request behavior.

src/rpc/core_rpc_server.cpp

contrib/epee/include/net/http_server_impl_base.h

vtnerd · 2025-02-02T20:08:02Z

Sorry about this obvious failure - only ran ad-hoc tests for this and it shows. Ill figure out a fix shortly.

iamamyth · 2025-02-02T20:49:47Z

The connection limit seems more nuanced and difficult. You may want to consider splitting the payload and connection limits into separate PRs, as the payload limit should be rather straightforward.

SyntheticBird45 · 2025-02-02T21:02:27Z

The connection limit seems more nuanced and difficult. You may want to consider splitting the payload and connection limits into separate PRs, as the payload limit should be rather straightforward.

I disagree, these are related enough, let's not waste time by splitting this PR. I would like by a miracle to see this merged into the incoming release.

0xFFFC0000

I would like to see programmatical way to set and get max_ip_connections.

I have been playing with this test case for this PR, but so far not able to be successful. The scaffold of the test is ready. and a single test is included too. You can add extra tests to it too :


diff --git a/tests/unit_tests/CMakeLists.txt b/tests/unit_tests/CMakeLists.txt
index e329b7506..1c94755f7 100644
--- a/tests/unit_tests/CMakeLists.txt
+++ b/tests/unit_tests/CMakeLists.txt
@@ -102,6 +102,7 @@ set(unit_tests_sources
   is_hdd.cpp
   aligned.cpp
   rpc_version_str.cpp
+  rpc_server.cpp
   zmq_rpc.cpp)
 
 set(unit_tests_headers
diff --git a/tests/unit_tests/rpc_server.cpp b/tests/unit_tests/rpc_server.cpp
new file mode 100644
index 000000000..cd574f483
--- /dev/null
+++ b/tests/unit_tests/rpc_server.cpp
@@ -0,0 +1,128 @@
+// Copyright (c) 2019-2024, The Monero Project
+// 
+// All rights reserved.
+// 
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+// 
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+//    conditions and the following disclaimer.
+// 
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+//    of conditions and the following disclaimer in the documentation and/or other
+//    materials provided with the distribution.
+// 
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+//    used to endorse or promote products derived from this software without specific
+//    prior written permission.
+// 
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "gtest/gtest.h"
+#include "rpc/core_rpc_server.h"
+#include "net/http_client.h"
+#include "net/http_server_impl_base.h"
+#include "net/http_protocol_handler.h"
+#include "net/net_utils_base.h"
+#include "cryptonote_core/cryptonote_core.h"
+#include "p2p/net_node.h"
+#include <thread>
+#include <chrono>
+#include <boost/program_options.hpp>
+
+// Create a test subclass of core_rpc_server to expose a public setter for response limit.
+class TestCoreRpcServer : public cryptonote::core_rpc_server {
+public:
+    using cryptonote::core_rpc_server::core_rpc_server; // inherit constructors
+
+    void set_test_response_soft_limit(std::size_t limit) {
+        // Expose the call to the underlying boosted_tcp_server's set_response_soft_limit
+        m_net_server.set_response_soft_limit(limit);
+    }
+};
+
+class RpcServerTest : public ::testing::Test {
+protected:
+    void SetUp() override { 
+        boost::program_options::options_description opts{};
+        nodetool::node_server<cryptonote::t_cryptonote_protocol_handler<cryptonote::core>>::init_options(opts);
+        cryptonote::core::init_options(opts);
+        cryptonote::core_rpc_server::init_options(opts);
+
+        const char* argv[] = {"monerod", "--rpc-bind-port", "18081", "--no-igd"};
+        int argc = 4;
+        boost::program_options::variables_map vm;
+        boost::program_options::store(
+          boost::program_options::parse_command_line(argc, argv, opts), vm
+        );  
+
+        // Initialize core and protocol handler
+        core = new cryptonote::core(nullptr);
+        protocol_handler = new cryptonote::t_cryptonote_protocol_handler<cryptonote::core>(*core, nullptr, false);
+
+        // Initialize node server with protocol handler
+        node_server = new nodetool::node_server<cryptonote::t_cryptonote_protocol_handler<cryptonote::core>>(*protocol_handler);
+
+        // Create the RPC server with core and node server using our test subclass.
+        server = new TestCoreRpcServer(*core, *node_server);
+
+        // Set response soft limit to 1 MB via the public method.
+        static_cast<TestCoreRpcServer*>(server)->set_test_response_soft_limit(1024 * 1024);
+
+        // Start the server on a separate thread
+        server_thread = std::thread([this, &vm]() {
+            server->init(vm, false, "", false, "");
+            server->run(1, true);
+        });
+
+        // Wait to ensure the server has started
+        std::this_thread::sleep_for(std::chrono::seconds(2));
+    }
+
+    void TearDown() override {
+        // Stop the server and join the thread
+        server->send_stop_signal();
+        if (server_thread.joinable())
+            server_thread.join();
+        delete server;
+        delete node_server;
+        delete protocol_handler;
+        delete core;
+    }
+
+    cryptonote::core_rpc_server* server;
+    cryptonote::core* core;
+    cryptonote::t_cryptonote_protocol_handler<cryptonote::core>* protocol_handler;
+    nodetool::node_server<cryptonote::t_cryptonote_protocol_handler<cryptonote::core>>* node_server;
+    std::thread server_thread;
+};
+
+TEST_F(RpcServerTest, ResponseSizeLimit) {
+    // Create an HTTP client to send requests to the RPC server.
+    epee::net_utils::http::http_simple_client http_client;
+    epee::net_utils::http::fields_list fields;
+
+    // Prepare a request expected to produce a large response.
+    std::string request_body = R"({"jsonrpc":"2.0","id":"0","method":"get_block_headers_range","params":{"start_height":0,"end_height":100000}})";
+
+    // The correct signature expects:
+    // (URI, method, body, timeout, response_info pointer, additional fields)
+    const epee::net_utils::http::http_response_info* response_info = nullptr;
+    bool success = http_client.invoke("http://127.0.0.1:18081/json_rpc", "POST",
+                                      request_body, std::chrono::milliseconds(10000),
+                                      &response_info, std::move(fields));
+
+    // Verify the HTTP call was successful.
+    ASSERT_TRUE(success);
+
+    // Verify the response body is within the configured 1 MB limit.
+    ASSERT_LE(response_info->m_body.size(), 1024 * 1024);
+}
\ No newline at end of file

vtnerd · 2025-02-03T22:39:23Z

@0xFFFC0000 are you hoping for a unit test for this? I think that's you meant.

selsta · 2025-02-03T23:23:09Z

Please also open again release branch, we will include it for v0.18.4.0.

src/rpc/core_rpc_server.cpp

nahuhh · 2025-02-04T13:49:49Z

src/rpc/core_rpc_server.cpp

+  const command_line::arg_descriptor<std::size_t> core_rpc_server::arg_rpc_max_ip_connections = {
+      "rpc-max-ip-connections"
+    , "Max RPC connections per IP permitted"
+    , 3


What about localhost or lan connections, typically coming from onion / i2p setups?

The next patch will have rpc-max-connections-per-public-ip and rpc-max-conections-per-private-ip. localhost connections will fall under the private-ip category.

BTW, I'm not really sure how to handle Tor RPC, its arguably broken.

Yeah not much can be done regarding it, public onion nodes will have to make their own load balancer and limiter.

I'd suggest a higher limit for private connections, somewhere in the 5-10 range. The functional tests might actually provide helpful guidance on a suitable limit, precisely because they have a flawed, but common, connection management strategy (i.e. just figure out whatever value works with that test suite, and maybe round it a bit).

I'd suggest a higher limit for private connections, somewhere in the 5-10 range. The functional tests might actually provide helpful guidance on a suitable limit, precisely because they have a flawed, but common, connection management strategy (i.e. just figure out whatever value works with that test suite, and maybe round it a bit).

force push just set max connection per private ip to 100.

Right, it's 100 in the test runs (which effectively passed, because the failure is a methodologically flawed test), but still 3 by default in the daemon. I suspect the limit in the tests can actually be lowered and synchronized with the daemon's default.

Oh you're right I misread.

vtnerd · 2025-02-06T17:47:33Z

Force pushed:

A fix in the epee TCP server where the socket wouldn't cleanup/close despite starting the terminate() sequence.
A flag separating maxes for private and public IPs were added. localhost connections are considered private.
Added a "kludge" where max private IPs is set to 0 for the functional tests. This works locally, will see about CI server
Fixed default value for response_soft_limit (from 0 to 25MiB).

If all tests pass, I will open on release branch.

vtnerd · 2025-02-06T17:48:57Z

Also @0xFFFC0000 I added two unit tests. I testing the http code directly instead of the method you were using. This helped ensure that the TCP server was acting as expected. This caught a difficult to catch/fix bug involving async op cancellations and ASIO composed handlers.

0xFFFC0000 · 2025-02-06T17:50:19Z

Also @0xFFFC0000 I added two unit tests. I testing the http code directly instead of the method you were using. This helped ensure that the TCP server was acting as expected. This caught a difficult to catch/fix bug involving async op cancellations and ASIO composed handlers.

Glad to hear that.

vtnerd · 2025-02-06T20:21:05Z

Another force push:

Changed limits to --rpc-max-connections-per-ip and --rpc-max-connections.
Hopefully fixed builds with older Boost releases (Beast issues).

As I side note that I hadn't mentioned previously: the new unit test uses boost::beast for testing. So we will also gain limited external verification for our http server.

vtnerd · 2025-02-07T00:29:54Z

I've had private discussions about an additional change to this patch. The change would have 3 flags: (1) rpc-max-connections-per-public-ip, (2) rpc-max-connections-per-private-ip, and (3) rpc-max-connections.

The desired goal is to set different limits for Tor/I2P connections. The per-private-ip achieves that, but has other downsides. I'm not going to add this without discussion, as I've already got an approval from one contributor.

tests/unit_tests/epee_http_server.cpp

contrib/epee/include/net/http_protocol_handler.inl

SyntheticBird45 · 2025-02-07T01:11:22Z

I've had private discussions about an additional change to this patch. The change would have 3 flags: (1) rpc-max-connections-per-public-ip, (2) rpc-max-connections-per-private-ip, and (3) rpc-max-connections.

The desired goal is to set different limits for Tor/I2P connections. The per-private-ip achieves that, but has other downsides. I'm not going to add this without discussion, as I've already got an approval from one contributor.

This 3 flag solution is best imo. I see a useful for reasoning for all these three.

selsta · 2025-02-07T17:58:47Z

Is it necessary to set the max RPC connections limit to 100? It would mean by default public RPC nodes would stop working at some point (though I don't know if they support 100 connections in the first place), and a single entity with 100 IPs could basically open connections to all public nodes and make them reject connections from legit useres.

SyntheticBird45 · 2025-02-07T18:03:02Z

Is it necessary to set the max RPC connections limit to 100? It would mean by default public RPC nodes would stop working (though I don't know if they support 100 connections in the first place), and a single entity with 100 IPs could basically open connections to all public nodes and make them reject connections from legit useres.

It's really about max resource usage and public nodes should absolutely tweak the rpc-soft-limit down to what is strictly necessary to increase max rpc connections. Unless i am stupid (I can be stupid) there are no responses under restricted RPC that go to 25MB

iamamyth · 2025-02-07T23:03:08Z

Is it necessary to set the max RPC connections limit to 100?

I don't know what precipitated the in-daemon connection limit (can be done with a firewall), but, one could argue that the principle of least surprise favors not auto-configuring a new limit (i.e. keeping unlimited as the default). The data limit, on the other hand, falls into a separate category (nothing should exceed it now, doing so creates stability problems, and the networking stack can't see data which hasn't been sent to the socket). This distinction is exactly why I suggested splitting the PR (one of these two things is just much trickier to get right on first attempt, because the chosen strategy itself could prove wrong), but, at this point, we're far enough down the road that it's no longer a consideration.

nahuhh · 2025-02-07T23:13:34Z

I don't know what precipitated the in-daemon connection limit (can be done with a firewall), but, one could argue that the principle of least surprise favors not auto-configuring a new limit (i.e. keeping unlimited as the default). The data limit, on the other hand, falls into a separate category (nothing should exceed it now, doing so creates stability problems, and the networking stack can't see data which hasn't been sent to the socket).

someone who controls subnets (like the spynodes) can flood 5 connections per ip. Thats 1280 connections for one /24 subnet.

currently its a potential 65k connections for 1 subnet.

@Gingeropolous might have good numbers for his rpc connections. Ive seen him report 73 etc before. @plowsof runs some popular nodes and doesnt have near 100 connections (less than 10 last check). Someone like cake, who runs a very high specced and high use node, would likely increase max to 1000 or greater.

SyntheticBird45 · 2025-02-07T23:30:50Z

@nahuhh Big public nodes like Cake Wallet uses load balancer/reverse http proxy which let them be very liberal on the limit because it's the proxy responsibility to ensure these limits.

vtnerd · 2025-02-08T00:10:20Z

Force pushed (twice):

Now have 3 flags
The unit tests for connection limit does a read/write sequence to ensure each connection works
Changed a min(...) call to if (foo) --foo
Reordered the addition after the operator[] call in case it throws.

Watching the CI before pushing to the other PR

Gingeropolous · 2025-02-08T00:18:06Z

@Gingeropolous might have good numbers for his rpc connections. Ive seen him report 73 etc before. @plowsof runs some popular nodes and doesnt have near 100 connections (less than 10 last check). Someone like cake, who runs a very high specced and high use node, would likely increase max to 1000 or greater.

Yeah, so my experience was having 70+ RPC connections, and at one point I found that there were multiple connections from the same IP address. This made the node unresponsive at times, and led me to create my incredibly hacky p2r2n_defender script:

https://github.com/Gingeropolous/p2r2n_defender/blob/main/blockddos_v1.sh

this PR will be great for public RPC nodes.

nahuhh · 2025-02-08T00:56:59Z

@Gingeropolous how much ram did the node have? and ssd or hdd?

src/wallet/wallet_rpc_server.cpp

Gingeropolous · 2025-02-08T02:06:05Z

@nahuhh , that was 64GB ram with sata ssd.

vtnerd · 2025-02-08T03:27:36Z

Force pushed fix for the functional tests. Sorry about that.

vtnerd · 2025-02-08T03:30:21Z

Force push for a fix for default values in the wallet server.

iamamyth · 2025-02-08T05:05:13Z

contrib/epee/include/net/http_protocol_handler.inl

+	    if (m_initialized)
+	    {
+	      CRITICAL_REGION_LOCAL(m_config.m_lock);
+        if (m_config.m_connection_count)


Minor style issue (not material for approval): If GitHub browser view is correct, the indent here appears to differ from elsewhere.

selsta added pending review networking labels Feb 2, 2025

SyntheticBird45 reviewed Feb 2, 2025

View reviewed changes

src/rpc/core_rpc_server.cpp Show resolved Hide resolved

SyntheticBird45 reviewed Feb 2, 2025

View reviewed changes

contrib/epee/include/net/http_server_impl_base.h Outdated Show resolved Hide resolved

0xFFFC0000 reviewed Feb 2, 2025

View reviewed changes

selsta mentioned this pull request Feb 3, 2025

v0.18.4.0 release to-do list #9758

Open

7 tasks

nahuhh reviewed Feb 4, 2025

View reviewed changes

vtnerd force-pushed the improve/rpc_threshold branch 2 times, most recently from b673ce9 to 704d0c1 Compare February 6, 2025 17:44

vtnerd force-pushed the improve/rpc_threshold branch from 704d0c1 to 0d7dc83 Compare February 6, 2025 20:19

vtnerd force-pushed the improve/rpc_threshold branch 2 times, most recently from 972bf95 to 2bad904 Compare February 6, 2025 20:36

SyntheticBird45 approved these changes Feb 6, 2025

View reviewed changes

vtnerd mentioned this pull request Feb 6, 2025

Set response limits on http server connections [0.18] #9775

Open

iamamyth reviewed Feb 7, 2025

View reviewed changes

tests/unit_tests/epee_http_server.cpp Show resolved Hide resolved

iamamyth reviewed Feb 7, 2025

View reviewed changes

contrib/epee/include/net/http_protocol_handler.inl Outdated Show resolved Hide resolved

vtnerd force-pushed the improve/rpc_threshold branch 2 times, most recently from c9976b6 to c39f00a Compare February 8, 2025 00:06

nahuhh reviewed Feb 8, 2025

View reviewed changes

src/wallet/wallet_rpc_server.cpp Outdated Show resolved Hide resolved

vtnerd force-pushed the improve/rpc_threshold branch from c39f00a to a29f476 Compare February 8, 2025 03:26

Set response limits on http server connections

f4189f4

vtnerd force-pushed the improve/rpc_threshold branch from a29f476 to f4189f4 Compare February 8, 2025 03:30

iamamyth approved these changes Feb 8, 2025

View reviewed changes

iamamyth reviewed Feb 8, 2025

View reviewed changes

Add configuration options to HTTP server #9765

Are you sure you want to change the base?

Add configuration options to HTTP server #9765

Conversation

vtnerd commented Feb 1, 2025

selsta commented Feb 2, 2025

selsta commented Feb 2, 2025

iamamyth commented Feb 2, 2025 • edited Loading

vtnerd commented Feb 2, 2025

iamamyth commented Feb 2, 2025

SyntheticBird45 commented Feb 2, 2025

0xFFFC0000 left a comment

Choose a reason for hiding this comment

vtnerd commented Feb 3, 2025

selsta commented Feb 3, 2025

Choose a reason for hiding this comment

vtnerd Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd commented Feb 6, 2025

vtnerd commented Feb 6, 2025

0xFFFC0000 commented Feb 6, 2025

vtnerd commented Feb 6, 2025

vtnerd commented Feb 7, 2025

SyntheticBird45 commented Feb 7, 2025

selsta commented Feb 7, 2025 • edited Loading

SyntheticBird45 commented Feb 7, 2025 • edited Loading

iamamyth commented Feb 7, 2025 • edited Loading

nahuhh commented Feb 7, 2025

SyntheticBird45 commented Feb 7, 2025

vtnerd commented Feb 8, 2025

Gingeropolous commented Feb 8, 2025

nahuhh commented Feb 8, 2025

Gingeropolous commented Feb 8, 2025

vtnerd commented Feb 8, 2025

vtnerd commented Feb 8, 2025

Choose a reason for hiding this comment

iamamyth commented Feb 2, 2025 •

edited

Loading

vtnerd Feb 5, 2025 •

edited

Loading

selsta commented Feb 7, 2025 •

edited

Loading

SyntheticBird45 commented Feb 7, 2025 •

edited

Loading

iamamyth commented Feb 7, 2025 •

edited

Loading